Skip to content

Examples refresh, take 1 #219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 68 commits into from
Mar 12, 2020
Merged

Examples refresh, take 1 #219

merged 68 commits into from
Mar 12, 2020

Conversation

mattsb42-aws
Copy link
Member

@mattsb42-aws mattsb42-aws commented Mar 9, 2020

Issue #, if available:
#156
#213
resolves: #177

Description of changes:

There are three main things going on here:

  1. Forcing every example author to also write a test is cumbersome. To get away from this, I reworked how we test the examples so that as long as the example function has the right name and signature (outlined in the new examples readme) it will Just Work.
  2. In preparation for all of the revised examples, I pulled all of the existing examples into a legacy module. I'll be pulling some of them out into their appropriate place later, pending the next item.
  3. I added some initial new-format examples to show how to use the high-level APIs. I want to make sure we like this format (guiding comments, layout, flow, etc) before writing the couple dozen more examples that we need so that I don't need to copy-paste the same changes into dozens of files.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

johnwalker and others added 30 commits July 15, 2019 11:08
* Added a check for max_age being greater than 0

* Fixed flake8 by adding missing pydocstyle dependency

* Added the dependency to decrypt_oracle as well

* Added test for max_age<=0 ValueError

* Updated test for max_age<=0.0 ValueError

* Added negative test case
… still working on populating the tests correctly, so the CI will fail, but I tested the code with my own KMS CMK ARNs, so I know it will work once the tests are populated (working with Tejeswini on this)
…d test keys in different regions for this example
…ured (#179)

* Fixed KMS master key provider tests for users who have their default AWS region configured

* created fixture for botocore session with no region set

* add auto-used fixture in KMS master key provider unit tests to test against both with and without default region
acioc
acioc previously approved these changes Mar 9, 2020
Copy link
Contributor

@juneb juneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got through the intro + streaming example. If you accept a change, please propagate that change throughout the file. I'll review again when you've rev'd the PR.

for chunk in encryptor:
ciphertext.write(chunk)

# Verify that the ciphertext and plaintext are different.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of a low bar ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, but I'm not sure what else we can really make a concrete statement about to demonstrate that "yes, this is encrypted" without getting really complicated.

@mattsb42-aws mattsb42-aws dismissed stale reviews from acioc via c54f21a March 9, 2020 23:55
Copy link
Contributor

@juneb juneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed until MK/MKP legacy examples. If you need more, let me know.

@mattsb42-aws mattsb42-aws requested review from acioc and juneb March 12, 2020 18:54
Copy link

@acioc acioc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@mattsb42-aws mattsb42-aws merged commit 9402826 into aws:keyring Mar 12, 2020
@mattsb42-aws mattsb42-aws deleted the keyring-examples-reorg branch March 12, 2020 19:03
WesleyRosenblum added a commit to aws/aws-encryption-sdk-java that referenced this pull request Mar 31, 2020
* Adding new examples and example test runner to follow the format set
in aws/aws-encryption-sdk-python#219

* Updated wording and copyright notice

* Adding periods

* Adding NIST recommendation for RSA

* Adding example for DER formatted RSA keys

* Wording changes based on feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants